Skip to content

fix(driver-podman): avoid panic when HOME is unset on macOS#2327

Open
mesutoezdil wants to merge 4 commits into
NVIDIA:mainfrom
mesutoezdil:fix/podman-socket-home-fallback
Open

fix(driver-podman): avoid panic when HOME is unset on macOS#2327
mesutoezdil wants to merge 4 commits into
NVIDIA:mainfrom
mesutoezdil:fix/podman-socket-home-fallback

Conversation

@mesutoezdil

@mesutoezdil mesutoezdil commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

default_socket_path used expect("HOME must be set on macOS"), which panics if HOME is not set (for example under minimal launchd/service contexts).

This falls back to the passwd entry for the current user (same approach as the dirs/home crates) instead of panicking. Only panics if neither HOME nor a passwd entry is available, which should not happen in practice.

No new dependency, nix is already a workspace dependency and already used this way elsewhere in the codebase.

@copy-pr-bot

copy-pr-bot Bot commented Jul 16, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@krishicks

Copy link
Copy Markdown
Collaborator

This is missing test coverage; would you add a test for it?

Also, would you mind expanding on this use case?

(for example under minimal launchd/service contexts)

@mesutoezdil
mesutoezdil force-pushed the fix/podman-socket-home-fallback branch from f9b5e52 to 1c6948e Compare July 16, 2026 20:39
@mesutoezdil

mesutoezdil commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

This is missing test coverage; would you add a test for it?

Also, would you mind expanding on this use case?

(for example under minimal launchd/service contexts)

sure

on launchd example I dont v a specific documented incident, that was a general illustration, not a real case I hit.
the actual motivation is that home is a shell/session convention..

and 2 ss for changes

Screenshot 2026-07-16 at 22 37 41 Screenshot 2026-07-16 at 22 38 45

@krishicks

krishicks commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

I think there's a better solution here, where we can fix this issue and also fix a latent bug, which is that presently the gateway can auto-detect Podman by probing known Podman sockets, but both the podman and vm drivers could fail to take advantage of the auto-detected socket because the drivers only check this $HOME path on macOS, and the podman driver happens to panic when $HOME is unset.

Basically, we should align the Podman socket selection with my recent change to the Docker socket selection (cf4decc). That change enhanced Docker socket selection but also made sure the client used the selected/auto-detected socket when no socket was defined in configuration. It's a bigger change, but I think it's much more valuable than changing this panic behavior.

The path I think we should take is to make Podman mirror Docker’s configuration model:

  1. Add a shared detector in openshell-core:
pub fn detect_podman_socket() -> Option<PathBuf>

It should probe podman_socket_candidates() and return the first responsive Podman socket. Then implement is_podman_available() using it.

  1. Change PodmanComputeConfig.socket_path:
pub socket_path: Option<PathBuf>

Its default becomes None, matching DockerComputeConfig. An explicit TOML, CLI, or environment value becomes Some(path).

  1. Resolve the socket once in PodmanComputeDriver::new():

(this removes the default because Podman has no default, really; the installation mechanism determines the default)

let socket_path = config
  .socket_path
  .clone()
  .or_else(openshell_core::config::detect_podman_socket)
  .ok_or_else(|| {
      PodmanApiError::InvalidInput(
          "no responsive Podman API socket found; \
           set OPENSHELL_PODMAN_SOCKET or configure socket_path"
              .to_string(),
      )
  })?;

let client = PodmanClient::new(socket_path.clone());
config.socket_path = Some(socket_path);

That gives the same precedence as Docker, but without the fallback:

explicit configuration
→ detected responsive socket
  1. Stop eagerly resolving the path elsewhere:
  • Standalone Podman CLI passes args.podman_socket directly into the config.
  • Server environment override assigns Some(PathBuf::from(...)).
  • TOML deserialization naturally produces Some(path) when configured.
  • PodmanComputeConfig::default() leaves it as None.
  1. Update the VM and podman drivers:
  • Remove podman_socket_path() (vm) and default_socket_path() (podman).
  • Use openshell_core::config::detect_podman_socket().
  • Connect Bollard to the returned path and ping it as it does today.
  1. Consolidate tests:
  • Core tests candidate ordering, overrides, stale sockets, and Podman response validation.
  • Podman driver tests explicit → detected precedence.
  • VM tests no longer test platform-specific path construction.

The resulting symmetry is:

DockerComputeConfig.socket_path: Option<PathBuf>
DockerComputeDriver::new(): explicit → detect_docker_socket → fallback

PodmanComputeConfig.socket_path: Option<PathBuf>
PodmanComputeDriver::new(): explicit → detect_podman_socket

That centralizes discovery in openshell-core and leaves each driver responsible only for constructing its client from the resolved socket.

If you want to take on this change, feel free, otherwise let me know and I can take it.

An added bonus would look for podman first (like it used to, before a different change of mine), but actually use it to run podman info, which includes the actual socket. This would resolve #1690, because Podman installed in different ways uses different sockets (including /var/run/docker.sock when using Podman Desktop).

Align Podman socket selection with the existing Docker model: explicit
config wins, otherwise probe openshell-core for a responsive socket.
This also fixes the original HOME-unset panic, since resolution no
longer hardcodes a per-OS default path.

- add detect_podman_socket() in openshell-core, mirroring
  detect_docker_socket
- PodmanComputeConfig.socket_path is now Option<PathBuf>, no default
- remove default_socket_path() (podman driver) and podman_socket_path()
  (vm driver), both replaced by the shared detector
- update server env override and CLI for the new Option type
- add tests: responsive-candidate detection in openshell-core, and
  config-error (not panic) when no socket is configured or reachable
@mesutoezdil
mesutoezdil force-pushed the fix/podman-socket-home-fallback branch from 1c6948e to 919e818 Compare July 17, 2026 05:35
@mesutoezdil

mesutoezdil commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

added detect_podman_socket() in openshell-core, made PodmanComputeConfig.socket_path an Option<PathBuf> resolved explicit-then-detected in PodmanComputeDriver::new(),
removed the duplicate default_socket_path()/podman_socket_path() logic in both drivers,
and added tests;
left the podman info bonus out as a possible follow-up.

Comment thread crates/openshell-driver-podman/src/config.rs Outdated
Comment thread crates/openshell-driver-podman/src/driver.rs Outdated
@krishicks

Copy link
Copy Markdown
Collaborator

Additional findings from gpt-5.6-sol medium:

  1. The new no-socket test depends on the host not running Podman.
    driver.rs:902

    The test redirects OPENSHELL_PODMAN_SOCKET, XDG_RUNTIME_DIR, and HOME to nonexistent paths, but on Linux the core candidate list still includes /run/user/actual-uid/podman/podman.sock. A developer running rootless Podman can therefore get a detected socket, causing the test to proceed instead of returning the expected error. It also mutates process-wide environment without coordinating with other environment-mutating tests.

    I’d extract socket resolution into a function accepting a detector, or otherwise inject the detection result. That would allow deterministic tests for:

    • Explicit socket wins.
    • Detected socket is used when configuration is absent.
    • Neither source produces the expected error.
  2. Documentation still describes a platform-default socket that no longer exists.
    Podman README configuration table

    The README says the default is $XDG_RUNTIME_DIR/... or $HOME/.... After this PR, the behavior is “probe candidates and fail if none respond.” The published compute-driver documentation also doesn’t explain the new behavior when socket_path is omitted.

    This PR changes a gateway TOML field’s type/default and user-visible startup behavior, so project instructions require updating at least:

    • crates/openshell-driver-podman/README.md
    • docs/reference/gateway-config.mdx
    • Likely docs/reference/sandbox-compute-drivers.mdx

Extract socket resolution into resolve_socket_path, taking the
detector as a parameter so tests do not depend on real env vars or
the host's actual Podman state. Replace the flaky env-mutating test
with three deterministic cases: explicit wins, detected is used when
absent, and neither source errors.

Fix the socket_path doc comment to describe it from a config user's
point of view, matching DockerComputeConfig's docstring. Drop a
comment that only made sense next to the Docker driver code.

Update the Podman README and gateway docs: they described a fixed
per-OS default path that no longer exists, replace with the actual
probe-then-fail behavior.
@mesutoezdil

Copy link
Copy Markdown
Contributor Author

Additional findings from gpt-5.6-sol medium:

  1. The new no-socket test depends on the host not running Podman.
    driver.rs:902
    The test redirects OPENSHELL_PODMAN_SOCKET, XDG_RUNTIME_DIR, and HOME to nonexistent paths, but on Linux the core candidate list still includes /run/user/actual-uid/podman/podman.sock. A developer running rootless Podman can therefore get a detected socket, causing the test to proceed instead of returning the expected error. It also mutates process-wide environment without coordinating with other environment-mutating tests.
    I’d extract socket resolution into a function accepting a detector, or otherwise inject the detection result. That would allow deterministic tests for:

    • Explicit socket wins.
    • Detected socket is used when configuration is absent.
    • Neither source produces the expected error.
  2. Documentation still describes a platform-default socket that no longer exists.
    Podman README configuration table
    The README says the default is $XDG_RUNTIME_DIR/... or $HOME/.... After this PR, the behavior is “probe candidates and fail if none respond.” The published compute-driver documentation also doesn’t explain the new behavior when socket_path is omitted.
    This PR changes a gateway TOML field’s type/default and user-visible startup behavior, so project instructions require updating at least:

    • crates/openshell-driver-podman/README.md
    • docs/reference/gateway-config.mdx
    • Likely docs/reference/sandbox-compute-drivers.mdx

all implemented

@krishicks

Copy link
Copy Markdown
Collaborator

/ok to test 39d8265

@krishicks

Copy link
Copy Markdown
Collaborator

Final nit from gpt-5.6-sol medium, which I agree with:

The README’s default description is self-contradictory and incomplete.

| `OPENSHELL_PODMAN_SOCKET` | `--podman-socket` | Auto-detected if unset: probes `$XDG_RUNTIME_DIR/podman/podman.sock` (Linux), the macOS Podman machine socket under `$HOME`, and this variable itself, using the first one that responds. Fails to start if none respond. | Podman API Unix socket path. |

It says “auto-detected if unset” but then says the detector probes “this variable itself.” If the variable is unset, it
contributes no candidate. It also omits /run/user/uid/podman/podman.sock.

Simpler and more durable wording would be:

Probes known local Podman API sockets and uses the first responsive socket. Fails to start if none respond.

Previous wording was self-contradictory (says auto-detect on unset,
then lists the same var as a probed candidate) and omitted the Linux
/run/user/uid/podman/podman.sock candidate.
@mesutoezdil

Copy link
Copy Markdown
Contributor Author

Final nit from gpt-5.6-sol medium, which I agree with:

The README’s default description is self-contradictory and incomplete.

| `OPENSHELL_PODMAN_SOCKET` | `--podman-socket` | Auto-detected if unset: probes `$XDG_RUNTIME_DIR/podman/podman.sock` (Linux), the macOS Podman machine socket under `$HOME`, and this variable itself, using the first one that responds. Fails to start if none respond. | Podman API Unix socket path. |

It says “auto-detected if unset” but then says the detector probes “this variable itself.” If the variable is unset, it contributes no candidate. It also omits /run/user/uid/podman/podman.sock.

Simpler and more durable wording would be:

Probes known local Podman API sockets and uses the first responsive socket. Fails to start if none respond.

hm yeah correct, done

@krishicks

Copy link
Copy Markdown
Collaborator

/ok to test 7e1ef86

@krishicks krishicks added the test:e2e Requires end-to-end coverage label Jul 17, 2026
@github-actions

Copy link
Copy Markdown

Label test:e2e applied for 7e1ef86. Open the existing run and click Re-run all jobs to execute with the label set. The run will execute the standard E2E suite after building the required gateway and supervisor images once. The matching required CI gate status on this PR will flip green automatically once the run finishes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:e2e Requires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants